Thanks for contributing. I would suggest a simpler approach. Just modify

into

 def main(): 
     command_name = sys.argv[1] 

     if command_name == "chat": 
         chat() 
     elif command_name == "env": 
         print_env() 
    else:
        train(command_name)

It should be enough, what do you think?

#2101
Hi @qgallouedec

Thanks for the feedback! I see your point about simplifying the approach. My initial thought was to keep the existing structure to maintain clarity for users regarding supported commands.

I mean it could be done in that straight forward way but as a user I would not know what commands I could use.

To find out I'd actually have to dig into the code and see what's gonna be executed e.g. how train works.

Maybe I am overcomplicating things here. This is my first contribution so I don't know what kinda users (weather technical enough or not) are using trl.

Either way, I will adjust the implementation based on what you think is best!

As a user I'd use

trl --help

We're currently tweaking sys.argv instead of a proper argparse. That's why the above command won't give anything.
But in the future, I'd like to use argparse instead.

@lewtun anything to say here maybe? You opened the issue and might have some additional suggestion.

trl --help wouldn't work and is currently also not working

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

trl --help wouldn't work and is currently also not working

In the future, we will probably move to subparser for the trl cli, so trl --help will output something.

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

Do you mind trying with the above suggestion? Also, the critical point here is to add the tests: like this one

trl --help wouldn't work and is currently also not working

In the future, we will probably move to subparser for the trl cli, so trl --help will output something.

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

Do you mind trying with the above suggestion? Also, the critical point here is to add the tests: like this one

trl/tests/test_cli.py

Lines 20 to 28 in 78249d9

	def test_sft_cli():
	try:
	subprocess.run(
	"trl sft --max_steps 1 --output_dir tmp-sft --model_name_or_path trl-internal-testing/tiny-random-LlamaForCausalLM --dataset_name stanfordnlp/imdb --learning_rate 1e-4 --lr_scheduler_type cosine --dataset_text_field text",
	shell=True,
	check=True,
	)
	except BaseException as exc:
	raise AssertionError("An error occured while running the CLI, please double check") from exc

Hi,

do you want me to add a test for each model or some dynamic way?

So if I understand correctly, you want to use the test you just proposed and this approach:

 def main(): 
     command_name = sys.argv[1] 

     if command_name == "chat": 
         chat() 
     elif command_name == "env": 
         print_env() 
    else:
        train(command_name)

?

do you want me to add a test for each model or some dynamic way?

For each model. The args may vary a lot so I don't think it's possible to have a generic test for all scripts

So if I understand correctly, you want to use the test you just proposed and this approach:

That's right